-
-
Notifications
You must be signed in to change notification settings - Fork 97
enhancement / address build command papercuts #153
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Co-authored-by: Simon Hamp <[email protected]>
Co-authored-by: Simon Hamp <[email protected]>
Co-authored-by: Simon Hamp <[email protected]>
… symlinked composer deps)
|
I'm sure this needs more polish, but I'd appreciate a review since I'm changing around quite a bit of stuff @simonhamp Don't want to make a wrong turn & have to backtrack. Appreciate it! 🙏🏻 |
|
Tests didn't pass for php 8.1 because I used some some incompatible syntax |
As long as the uploader is using the development
Yes. With a pretty short lifespan, so that logs don't hang around too long.
I think in the majority case, whatever the developer was using it for should have been done before the build occurs, so I think there's a good case for just removing it all. That said, it's not always so clear. I think we need to offload some of this to the developer to consider and provide appropriate config options to make it easy for them to define the behaviour they require. I also think
See my note in my review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great. I haven't tested yet, just providing some feedback as requested
|
Thanks for the review 👍 Sidenote. How do you feel about using the Laravel prompts spinner to give the build command some extra oomph? |
|
Sounds fine but we need to check it falls back to something reasonable on Windows |
|
I will try to review this properly in the next few days, but overall it feels great @gwleuverink, nice work! @SRWieZ re build directory: I agree. Let's do it, let's bring builds out of |
|
@SRWieZ thanks for your feedback! I definetly noticed some ide slowdowns and crashes. Let's pick that up in the next PR. I'll go through your other points (php-bin & moving app name) next. There seems to be one test that won't run in windows. But otherwise this PR is good to go |
|
Note: the boot test doesn't run also. But I suspect it's unrelated to this PR. |
|
Okay. We're here. Couple things to check - both unrelated to this PR.
Otherwise I think this is good to go. |
|
Note: We need to double check if the change Pete made in PR #162 isn't overwritten by merging this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Already approved, just tested it again.
Approved again!
|
@simonhamp since you're the original reviewer, I would appreciate your final approval when you have time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking great!
This PR adresses a couple of papercuts I've found in the build/publish commands.
Fixed
I've addressed this by porting the app copy logic to a php trait. This way the app is copied over before the electron-builder is executed. The copy step inside the electron-builder has been removed in favor for this approach. This way we don't have to modify any userland code.
During the copy step I account for files and directories that should be skipped. I've used an internal list here, which merges with the list of globs in the nativephp config file.
mac-latest.yamlwas overwritten by the last commandI've tried fixing that before in PR #145. Sadly this caused the last architecture not to compile with the correct php version
I've addressed this by removing the php building step as from the package.json scripts, and rather embed them in the build process by using a beforePack hook.
Other points for your consideration
GITHUB_REPO,GITHUB_OWNER,GITHUB_PRIVATEare cleaned up, the publish command cannot upload to github.They are all caught by the
GITHUB_*wildcard. I assume this assume is true for the other update providers too. Maybe the default list of env keys to be cleaned should be more specific, like:GITHUB_TOKEN? (that's the only important one that needs to be cleared anyway)I'm not sure if this still is an issue after these changes. Since we're not directly updating the projects' main env file anymore.
Should we default to
LOG_STACK=dailywhen building the app?node_modulesdirectory when building?This can become unwieldy quick! A fresh Laravel install already comes with a 30mb
node_modulesdirectory.Not sure if we want to keep
node_modules/.binin this case.. An argument could be made to keep that around.Not sure if this is by design.
php.jswas invoked with the--x86flag, while electron builder was invoked with--mac --x64in thebuild:mac-x86script.I kept this behaviour in, but not sure why it is like this